Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

supplement Postgres listen example with a small chat example #2577

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

JockeM
Copy link
Contributor

@JockeM JockeM commented Jul 1, 2023

Closes #161

this is just a continuation of #2165 made by Stephen
i just took his chat implementation and fixed the readme and renamed the folder to chat

@JockeM JockeM force-pushed the replace-example-listen-with-chat branch 2 times, most recently from 6fa7913 to ed8bab0 Compare July 4, 2023 12:43
Copy link
Collaborator

@abonander abonander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also adjust the Github Actions workflow that tests this example?

You can just update the entry for the listen example: https://github.com/launchbadge/sqlx/blob/main/.github/workflows/examples.yml#L184-L196

Those entries are alphabetized to make it easier to navigate that file, so this should also be moved above Files (Setup) to keep things organized.

@JockeM JockeM force-pushed the replace-example-listen-with-chat branch from ed8bab0 to 0553981 Compare July 9, 2023 22:00
@JockeM JockeM requested a review from abonander July 10, 2023 08:22
Copy link
Collaborator

@abonander abonander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is failing CI because it's expecting a TTY and not getting one, which is a real issue for automated testing. You'd have to specially allocate a TTY for the process, I think. But even so, it'd just sit there forever unless you actually sent it commands.

If you want to figure out how to automate TUI testing in CI, that's your prerogative, but I think it's good enough if we just check that it compiles. That should be noted in the README in case someone tries it and has problems with it.

That does make me hesitant to delete the listen example, however, as that was able to run in CI just fine. There's no reason not to have both, I think, since it's all handled in the same CI job anyway.

examples/postgres/chat/src/main.rs Outdated Show resolved Hide resolved
examples/postgres/chat/src/main.rs Outdated Show resolved Hide resolved
examples/postgres/chat/src/main.rs Outdated Show resolved Hide resolved
examples/postgres/chat/src/main.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@JockeM JockeM force-pushed the replace-example-listen-with-chat branch from 0553981 to d47531e Compare July 11, 2023 22:08
@JockeM
Copy link
Contributor Author

JockeM commented Jul 11, 2023

Yeah rather not go for the testing in CI for this.
Should I take back the listen example?

@abonander
Copy link
Collaborator

Yeah just keeping the listen example is fine @JockeM, thanks.

Copy link
Collaborator

@abonander abonander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One remaining nit + undeleting listen example.

.github/workflows/examples.yml Outdated Show resolved Hide resolved
Co-authored-by: Stephen <webmaster@scd31.com>
@JockeM JockeM force-pushed the replace-example-listen-with-chat branch from d47531e to 0b55ac5 Compare July 14, 2023 21:46
@JockeM JockeM requested a review from abonander July 15, 2023 18:43
@abonander abonander changed the title replace listen example with a small chat example supplement Postgres listen example with a small chat example Jul 24, 2023
@abonander abonander merged commit 8cad54c into launchbadge:main Jul 24, 2023
@scd31
Copy link

scd31 commented Jul 25, 2023

Sorry for missing the request on the original PR! It slipped through on my end. Glad to see the changes were made and this was merged! (:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the "listen" example and replace with a tiny CLI chat
3 participants